-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Theme Check] Add allowedDomains check for RemoteAsset theme checker #711
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
charlespwd
reviewed
Jan 16, 2025
charlespwd
reviewed
Jan 16, 2025
charlespwd
reviewed
Jan 17, 2025
3c85c95
to
21fbba9
Compare
charlespwd
approved these changes
Feb 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Patch could be minor but patch is fine too
Enhances developer experience by validating the remote assets (scripts, stylesheets) are loaded not only from Shopify CDNs but now approved domains. This prevents potential performance risks and incorrect errors from loading resources from untrusted sources, forcing developers to go and enable a source if it's _required_ The implementation includes: - Schema property for configuring allowedDomains - Domain validation against configured allow list - Test coverage for both allowed and non-allowed domains Reverted minor changeset to patch bump Fixed prettier issues Refactor domain list from global to parameter Changes domain validation to accept allowedDomains as a parameter instead of mutating a global array. This improves code clarity and testability by making data flow explicit. The change reduces side effets by: - Removing relince on shared mutable state - Making dpendencies clear through function signatures - Enabling better unit testing of domain validation logic Added file formatting Add normalisaztion & tests. - Added normalization to domains - Added extra tests to test against and for normalisation of strings. Update domains in test refactor: remove redundant regex validation in normaliseAllowedDomains - Remove regex pre-validation since new URL() already enforces strict URL standards - Simplify code by relying solely on new URL() for validation and normalisation Fixed issue where malformed URLs were throwing type errors. The test case urls were malformed as expected, but the new URL constructor was throwing Type Errors, which is expected if not wrapped in a try...catch block. Fixed by wrapping in a try...catch and returning false when malformed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'll update the PR template based on the revised commit message that focuses on developer experience and performance:
What are you adding in this PR?
Fixes #410
This PR adds domain validation to the RemoteAsset theme checker to ensure remote assets (scripts, stylesheets) are loaded from Shopify CDNs and approved domains. When a remote asset is referenced from an unlisted domain, the checker will provide guidance to help developers optimize performance and follow best practices.
Key changes:
allowedDomains
configuration in RemoteAsset check schemaWhat's next? Any followup issues?
Potential follow-ups:
What did you learn?
The variety of ways developers include remote assets highlighted the need for clear guidance on performance best practices. The test cases helped identify common patterns in how external resources are referenced in themes.
Before you deploy
changeset
allChecks
array insrc/checks/index.ts
yarn build
and committed the updated configuration filestheme-app-extension.yml
config (N/A - performance check applies to all themes)changeset